doc: fix http.ClientRequest method descriptions #15163

antoine-amara · 2017-09-03T13:43:30Z

fix documentation for methods getHeader, setHeader and removeHeader
for http.ClientRequest class. The documentation said these functions
can be called but they're wasn't describe into the API description yet.

add parameters and general description for each methods.

Fixes: #15048

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

benjamingr

I'm not sure the terminology of looking at request headers as "technically queued" is useful when explaining the concept.

That said, it's what we're currently doing in HTTP1 responses so I'm not going to block on it.

Could you please change the text in:

The header is still mutable using the setHeader(name, value), getHeader(name), removeHeader(name) API. The actual header will be sent along with the first data chunk or when closing the connection.

to links to the new method?

benjamingr · 2017-09-03T14:16:31Z

doc/api/http.md

+* `name` {string}
+* `value` {string}
+
+Sets a single header value for implicit headers. If this header already exists in the to-be-sent headers, its value will be replaced. Use an array of strings here if you need to send multiple headers with the same name.


I'm not sure the meaning of 'for implicit headers' is clear enough.

benjamingr · 2017-09-03T14:16:54Z

doc/api/http.md

+
+* `name` {string}
+
+Removes a header that's queued for implicit sending.


I don't think 'queued for implicit sending' is terminology I want to expose our users to.

benjamingr · 2017-09-03T14:17:26Z

doc/api/http.md

+* `name` {string}
+* Returns: {string}
+
+Reads out a header that's already been queued. Note that the name is case insensitive.


'Reads out a header on the request' maybe?

antoine-amara · 2017-09-04T20:56:52Z

@benjamingr, I commit new changes, thanks for your first review.

jasnell · 2017-09-07T05:52:13Z

doc/api/http.md

+* `name` {string}
+* `value` {string}
+
+Sets a single header value for headers object. If this header already exists in the to-be-sent headers, its value will be replaced. Use an array of strings here if you need to send multiple headers with the same name.


couple of things here:

This is a long line. Please line wrap at <= 80 chars

Please avoid the use of you in the docs.

jasnell

thank you for this! have a couple of things that need fixing!

BridgeAR · 2017-09-09T20:17:11Z

I think everything got addressed

antoine-amara · 2017-09-13T15:36:22Z

@BridgeAR, @jasnell, @benjamingr , should I rebase master and push the new commits into this branch before the next review ?

jasnell · 2017-09-13T15:41:38Z

rebasing is always a good idea! :-)

antoine-amara · 2017-09-13T15:44:14Z

ok so I rebase before the merge, I leave a comment after I push

antoine-amara · 2017-09-13T16:27:04Z

Ok, @BridgeAR, @jasnell, @benjamingr, the branch is up to date :) thanks for your reviews.

fix documentation for methods getHeader, setHeader and removeHeader for http.ClientRequest class. The documentation said these functions can be called but they're wasn't describe into the API description yet. add parameters and general description for each methods. Fixes: nodejs#15048

fix documentation for methods getHeader, setHeader and removeHeader for http.ClientRequest class. The documentation said these functions can be called but they're wasn't describe into the API description yet. add parameters and general description for each methods. PR-URL: #15163 Fixes: #15048 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

jasnell · 2017-09-15T22:43:55Z

Landed in ca2c73c

fix documentation for methods getHeader, setHeader and removeHeader for http.ClientRequest class. The documentation said these functions can be called but they're wasn't describe into the API description yet. add parameters and general description for each methods. PR-URL: nodejs/node#15163 Fixes: nodejs/node#15048 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

fix documentation for methods getHeader, setHeader and removeHeader for http.ClientRequest class. The documentation said these functions can be called but they're wasn't describe into the API description yet. add parameters and general description for each methods. PR-URL: #15163 Fixes: #15048 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

fix documentation for methods getHeader, setHeader and removeHeader for http.ClientRequest class. The documentation said these functions can be called but they're wasn't describe into the API description yet. add parameters and general description for each methods. PR-URL: nodejs/node#15163 Fixes: nodejs/node#15048 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

doc: fix http.ClientRequest method descriptions fix documentation for methods getHeader, setHeader and removeHeader for http.ClientRequest class. The documentation said these functions can be called but they're wasn't describe into the API description yet. add parameters and general description for each methods. PR-URL: nodejs#15163 Fixes: nodejs#15048 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> assert: use Same-value equality in deepStrictEqual PR-URL: nodejs#15398 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> formatting fix and scar deleted

nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Sep 3, 2017

benjamingr reviewed Sep 3, 2017

View reviewed changes

antoine-amara force-pushed the doc-#15048-clientrequest-documentation-inconsistencies branch from cbbd33e to 6670c2b Compare September 4, 2017 20:55

jasnell reviewed Sep 7, 2017

View reviewed changes

jasnell requested changes Sep 7, 2017

View reviewed changes

antoine-amara force-pushed the doc-#15048-clientrequest-documentation-inconsistencies branch 2 times, most recently from 6670c2b to 44c0877 Compare September 7, 2017 12:55

BridgeAR approved these changes Sep 9, 2017

View reviewed changes

jasnell approved these changes Sep 13, 2017

View reviewed changes

antoine-amara force-pushed the doc-#15048-clientrequest-documentation-inconsistencies branch from 44c0877 to bcdc178 Compare September 13, 2017 16:24

antoine-amara force-pushed the doc-#15048-clientrequest-documentation-inconsistencies branch from bcdc178 to adc1d28 Compare September 14, 2017 18:18

jasnell closed this Sep 15, 2017

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

antoine-amara deleted the doc-#15048-clientrequest-documentation-inconsistencies branch September 28, 2017 08:08

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: fix http.ClientRequest method descriptions #15163

doc: fix http.ClientRequest method descriptions #15163

antoine-amara commented Sep 3, 2017 •

edited

Loading

benjamingr left a comment

benjamingr Sep 3, 2017

benjamingr Sep 3, 2017

benjamingr Sep 3, 2017

antoine-amara commented Sep 4, 2017

jasnell Sep 7, 2017

jasnell left a comment

BridgeAR commented Sep 9, 2017

antoine-amara commented Sep 13, 2017

jasnell commented Sep 13, 2017

antoine-amara commented Sep 13, 2017

antoine-amara commented Sep 13, 2017

jasnell commented Sep 15, 2017


		* `name` {string}

		Removes a header that's queued for implicit sending.

doc: fix http.ClientRequest method descriptions #15163

doc: fix http.ClientRequest method descriptions #15163

Conversation

antoine-amara commented Sep 3, 2017 • edited Loading

Checklist

Affected core subsystem(s)

benjamingr left a comment

Choose a reason for hiding this comment

benjamingr Sep 3, 2017

Choose a reason for hiding this comment

benjamingr Sep 3, 2017

Choose a reason for hiding this comment

benjamingr Sep 3, 2017

Choose a reason for hiding this comment

antoine-amara commented Sep 4, 2017

jasnell Sep 7, 2017

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

BridgeAR commented Sep 9, 2017

antoine-amara commented Sep 13, 2017

jasnell commented Sep 13, 2017

antoine-amara commented Sep 13, 2017

antoine-amara commented Sep 13, 2017

jasnell commented Sep 15, 2017

antoine-amara commented Sep 3, 2017 •

edited

Loading